fix: URL-encode special characters in connection string passwords#597
Merged
anandgupta42 merged 2 commits intoAltimateAI:mainfrom Apr 5, 2026
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/drivers/src/normalize.ts (1)
161-163: PotentialURIErrorif username contains malformed percent sequences.
decodeURIComponent(user)will throw aURIErrorif the username contains malformed percent-encoded sequences (e.g.,user%GGname). While rare, this could cause unexpected failures.Consider wrapping in try-catch or skipping the decode/re-encode cycle for the username when it doesn't contain
%:🛡️ Proposed defensive fix
- // Re-encode both user and password to be safe - const encodedUser = encodeURIComponent(decodeURIComponent(user)) + // Re-encode both user and password to be safe. + // If user contains %, attempt decode; otherwise encode directly. + let encodedUser: string + try { + encodedUser = encodeURIComponent(decodeURIComponent(user)) + } catch { + // Malformed percent sequence — encode as-is + encodedUser = encodeURIComponent(user) + } const encodedPassword = encodeURIComponent(password)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/drivers/src/normalize.ts` around lines 161 - 163, The decode/re-encode of the username using decodeURIComponent(user) can throw a URIError for malformed percent sequences; update the logic around encodedUser (where decodeURIComponent(user) and encodeURIComponent are used) to first check if user contains '%' and only perform decode/encode when safe, or wrap decodeURIComponent(user) in a try-catch and fall back to using the original user string if decoding fails, ensuring encodedUser is always set to a valid encodeURIComponent value without throwing.packages/opencode/test/altimate/driver-normalize.test.ts (1)
974-978: Consider adding test coverage for/and?in passwords.The PR objectives mention handling
/as a special character, and the implementation's regex also includes?. Adding explicit tests would strengthen coverage:📝 Suggested additional tests
test("encodes / in password", () => { const input = "postgresql://admin:pass/word@localhost/db" const result = sanitizeConnectionString(input) expect(result).toBe("postgresql://admin:pass%2Fword@localhost/db") }) test("encodes ? in password", () => { const input = "postgresql://admin:pass?word@localhost/db" const result = sanitizeConnectionString(input) expect(result).toBe("postgresql://admin:pass%3Fword@localhost/db") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/driver-normalize.test.ts` around lines 974 - 978, Add two unit tests to the existing "encodes multiple special characters" suite that verify sanitizeConnectionString correctly percent-encodes '/' and '?' inside the password portion: create one test using input "postgresql://admin:pass/word@localhost/db" expecting "postgresql://admin:pass%2Fword@localhost/db" and another using "postgresql://admin:pass?word@localhost/db" expecting "postgresql://admin:pass%3Fword@localhost/db"; place these tests alongside the existing tests in driver-normalize.test.ts to ensure sanitizeConnectionString handles both characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/drivers/src/normalize.ts`:
- Around line 161-163: The decode/re-encode of the username using
decodeURIComponent(user) can throw a URIError for malformed percent sequences;
update the logic around encodedUser (where decodeURIComponent(user) and
encodeURIComponent are used) to first check if user contains '%' and only
perform decode/encode when safe, or wrap decodeURIComponent(user) in a try-catch
and fall back to using the original user string if decoding fails, ensuring
encodedUser is always set to a valid encodeURIComponent value without throwing.
In `@packages/opencode/test/altimate/driver-normalize.test.ts`:
- Around line 974-978: Add two unit tests to the existing "encodes multiple
special characters" suite that verify sanitizeConnectionString correctly
percent-encodes '/' and '?' inside the password portion: create one test using
input "postgresql://admin:pass/word@localhost/db" expecting
"postgresql://admin:pass%2Fword@localhost/db" and another using
"postgresql://admin:pass?word@localhost/db" expecting
"postgresql://admin:pass%3Fword@localhost/db"; place these tests alongside the
existing tests in driver-normalize.test.ts to ensure sanitizeConnectionString
handles both characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 44b3dc65-0c0f-455e-ab26-0eacd895c951
📒 Files selected for processing (3)
packages/drivers/src/index.tspackages/drivers/src/normalize.tspackages/opencode/test/altimate/driver-normalize.test.ts
VJ-yadav
force-pushed
the
fix/special-char-passwords
branch
2 times, most recently
from
ea99829 to
40886f2
Compare
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
cc @VJ-yadav |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/drivers/src/normalize.ts`:
- Around line 150-152: The current early-return on /%[0-9A-Fa-f]{2}/ prevents
addressing mixed raw/encoded passwords (e.g., "myp%40ss@word"); instead of
returning connectionString when that regex matches, decode the password (use
decodeURIComponent on the extracted password), then unconditionally re-encode
the decoded value (via encodeURIComponent) and substitute that encodedPassword
back into connectionString; remove the early-return on the regex, ensure you
operate on the password variable (or decodedPassword) and write the encoded
result into the connection string where the password is inserted (the code paths
around the existing password, connectionString and the later encoding
replacement on line ~170).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be01ceb1-6695-4d50-b1c0-f0cfb65f47ed
📒 Files selected for processing (3)
packages/drivers/src/index.tspackages/drivers/src/normalize.tspackages/opencode/test/altimate/driver-normalize.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/drivers/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/altimate/driver-normalize.test.ts
Contributor
Review — Ready for mergeScope: Fixes user-reported bug #589 — stored passwords with special characters ( Analysis
Files changed (3): CI status (commit
Minor observations (non-blocking)
Recommendation cc @anandgupta42 — ready for |
3 tasks
anandgupta42
force-pushed
the
fix/special-char-passwords
branch
from
bef2865 to
aa2d643
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/drivers/src/normalize.ts (1)
150-152:⚠️ Potential issue | 🟠 MajorRemove
%XXearly return; handle mixed encoded/raw passwords.Line 152 exits too early when any encoded byte is present. Passwords with both encoded and raw special chars remain partially unencoded and still break parsing.
Suggested fix
- // If the password already contains percent-encoded sequences, assume - // the caller encoded it properly and leave it alone. - if (/%[0-9A-Fa-f]{2}/.test(password)) return connectionString + let decodedPassword = password + try { + decodedPassword = decodeURIComponent(password) + } catch { + // malformed percent sequence; treat as raw + decodedPassword = password + } - const needsEncoding = /[@:`#/`?[\]%]/.test(password) - if (!needsEncoding) return connectionString + const needsEncoding = /[@:`#/`?[\]%]/.test(decodedPassword) + if (!needsEncoding && decodedPassword === password) return connectionString ... - const encodedPassword = encodeURIComponent(password) + const encodedPassword = encodeURIComponent(decodedPassword)Also applies to: 158-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/drivers/src/normalize.ts` around lines 150 - 152, Remove the early return that bails out when /%[0-9A-Fa-f]{2}/ matches and instead transform the password so existing percent-encoded triplets are preserved while any raw special characters are percent-encoded; specifically update the logic that currently checks the regex against password (the if (/%[0-9A-Fa-f]{2}/) return connectionString) to iterate the password string, copy any "%XX" sequences unchanged and percent-encode other characters (e.g., via encodeURIComponent on single chars or a small stateful loop), then reconstruct connectionString with the safely-encoded password; apply the same change to the other similar block around the later password-handling code (the 158-170 region) so mixed encoded/raw passwords are fully handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/drivers/src/normalize.ts`:
- Around line 136-138: The regex used to split userinfo from host in
normalize.ts (variable uriMatch matching connectionString) is greedy and can
cross into query/fragment when those contain '@'; update the pattern so the
userinfo capture is constrained to the authority segment (stop at '/', '?' or
'#'). Concretely, replace the (.+) group in the existing regex with a character
class that excludes '/', '?', and '#' (e.g., ([^\/?#]+)) so uriMatch only
considers the authority portion of connectionString when locating the '@'.
---
Duplicate comments:
In `@packages/drivers/src/normalize.ts`:
- Around line 150-152: Remove the early return that bails out when
/%[0-9A-Fa-f]{2}/ matches and instead transform the password so existing
percent-encoded triplets are preserved while any raw special characters are
percent-encoded; specifically update the logic that currently checks the regex
against password (the if (/%[0-9A-Fa-f]{2}/) return connectionString) to iterate
the password string, copy any "%XX" sequences unchanged and percent-encode other
characters (e.g., via encodeURIComponent on single chars or a small stateful
loop), then reconstruct connectionString with the safely-encoded password; apply
the same change to the other similar block around the later password-handling
code (the 158-170 region) so mixed encoded/raw passwords are fully handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66bfd234-ca6a-4056-a795-2527aead9488
📒 Files selected for processing (3)
packages/drivers/src/index.tspackages/drivers/src/normalize.tspackages/opencode/test/altimate/driver-normalize.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/drivers/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/altimate/driver-normalize.test.ts
Fixes AltimateAI#589 — stored passwords with special characters (`@`, `#`, `:`, `/`) cause cryptic authentication failures when used in URI-style connection strings because the userinfo section wasn't being percent-encoded. Root cause: - `sanitizeConnectionString()` existed in `normalize.ts` but was never wired into `normalizeConfig()` — connection strings with unencoded passwords went directly to drivers - The userinfo regex split on the *first* `@` instead of the *last*, so passwords containing `@` were silently mis-parsed and the function returned early thinking no encoding was needed Fix: - Wire `sanitizeConnectionString()` into `normalizeConfig()` as a single integration point so every URI-based driver benefits automatically - Fix the regex to use greedy matching for the userinfo portion and split on the last `@` before the host - Leave already-encoded passwords (`%XX` sequences) untouched for idempotency; decodeURIComponent wrapped in try/catch for safety Non-URI connection strings (Oracle TNS, key=value) and drivers using individual config fields (host/user/password) are unaffected — the fix only touches `connection_string`. Tests cover @/#/:/ in passwords, already-encoded passthrough, multiple URI schemes (postgresql://, mongodb://, mongodb+srv://), and the connectionString alias path.
anandgupta42
force-pushed
the
fix/special-char-passwords
branch
from
aa2d643 to
258a4cb
Compare
Multi-model code review flagged three real bugs in sanitizeConnectionString: 1. CRITICAL — greedy regex corrupted URIs with '@' in query/path Example: postgresql://u:p@host/db?email=alice@example.com Rightmost '@' was in the query string, not the userinfo separator. The sanitizer treated host/db?email=alice as part of the password and percent-encoded it, producing a broken URI. 2. MAJOR — short-circuit on %XX skipped passwords with mixed encoding Example: postgresql://user:p%20ss@word@host/db Partially-encoded password contained both a valid %20 and a raw '@'. The 'already-encoded, leave alone' check bailed early and left '@' unencoded. 3. MINOR — username-only userinfo (no colon) was skipped entirely Example: postgresql://alice@example.com@host/db Email-as-username with unencoded '@' broke URI parsing because we returned early on missing colon. Changes: - Scan for the LAST '@' in the afterScheme portion directly instead of first parsing out an 'authority' segment. The URI spec disallows unencoded path/query/fragment delimiters in authority, but real-world passwords do contain them — so we honor user intent over spec. - Replace the %XX short-circuit + needsEncoding pre-check with a single idempotent encoder (decode → encode) applied to both user and password components. This round-trips already-encoded values unchanged and encodes raw values correctly in one pass. - Handle username-only userinfo by encoding it when no colon is present. - Add an ambiguity guard: when the rightmost '@' is followed by no path/query/fragment delimiter but preceded by one, the '@' is in the query/path/fragment — return the URI unchanged and expect the caller to pre-encode. Test coverage added for all three bugs plus IPv6 hosts, URIs with no port, URIs with no path, path-with-@, fragment-with-@, and the ambiguous both-password-and-query-have-@ case.
Contributor
Consensus code-review applied — fixes pushedRan a 4-participant consensus review (Claude + GPT 5.4 + Gemini 3.1 Pro + self-review) and applied all flagged fixes. Real bugs found and fixed1. CRITICAL — greedy regex corrupted URIs with
2. MAJOR —
3. MINOR — username-only userinfo skipped entirely (Gemini)
Fix approach
Test coverage added11 new tests covering: 134 driver-normalize tests pass. TypeScript clean. All CI green. Pushed as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #589 — stored passwords with special characters (
@,#,:,/, etc.) cause cryptic authentication failures when used in URI-style connection strings.sanitizeConnectionString()existed innormalize.tsbut was never called — connection strings with unencoded passwords were passed directly to drivers@instead of the last, so passwords containing@were silently mis-parsed (the function returned early thinking no encoding was needed)sanitizeConnectionString()intonormalizeConfig()so every driver that accepts aconnection_stringbenefits automatically, and fix the regex to use greedy matching for the userinfo portionDrivers using individual config fields (host, user, password as separate values) are unaffected — native driver libraries handle raw passwords correctly without URI encoding.
Test Plan
sanitizeConnectionStringdirectly andnormalizeConfigintegration@,#,:are correctly percent-encoded%40,%23) are left untouchedmongodb://,mongodb+srv://,postgresql://schemes all handledconnection_string(individual fields) is not alteredconnectionStringalias resolves toconnection_stringbefore sanitizationChecklist
tsgoerror on@clickhouse/clientimport is unrelated (reproduces onmain)Summary by CodeRabbit
New Features
Tests